infosync: integrate PD HTTP client into the label manager #48738
Conversation
ti-chi-bot
bot
added
release-note-none
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48738 +/- ##
================================================
+ Coverage 71.0864% 72.8617% +1.7752%
================================================
Files 1365 1392 +27
Lines 404572 413041 +8469
================================================
+ Hits 287596 300949 +13353
+ Misses 97010 93148 -3862
+ Partials 19966 18944 -1022
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JmPotato
force-pushed
the
adopt_pd_http_cli_to_infosyncer
branch
2 times, most recently
from
6d64793 to
7a7796b
Compare
| tikv.WithPDHTTPClient([]string{ts.Addr()}), | ||
| ), | ||
| ) | ||
| ts.store, err = mockstore.NewMockStore() |
There was a problem hiding this comment.
The previous commit made changes to this code section, but it was later determined to be unnecessary during testing. This PR reverts those changes.
0f35b6f#diff-69c54bd9e8a29f957a0cb3d46941079117eadecfa6090abfac3bd05f78ec37e1L453
Signed-off-by: JmPotato <[email protected]>
Signed-off-by: JmPotato <[email protected]>
Signed-off-by: JmPotato <[email protected]>
Signed-off-by: JmPotato <[email protected]>
JmPotato
force-pushed
the
adopt_pd_http_cli_to_infosyncer
branch
from
7a7796b to
fb36395
Compare
|
@JmPotato: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/test check-dev2 |
|
@JmPotato: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test unit-test |
|
@JmPotato: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -209,7 +209,10 @@ func GlobalInfoSyncerInit( | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| is.labelRuleManager = initLabelRuleManager(etcdCli) | |||
| if pdHTTPCli != nil { | |||
| pdHTTPCli = pdHTTPCli.WithRespHandler(pdResponseHandler) | |||
There was a problem hiding this comment.
Could we put it to initLabelRuleManager? It seems we use pdHTTPCli only in initLabelRuleManager.
There was a problem hiding this comment.
Later we will integrate the PD HTTP client into the Placement Rule manager below also, so I keep it initialized here.
| func NewRulePatch(setRules []*Rule, deleteRules []string) *pd.LabelRulePatch { | ||
| labelRules := make([]*pd.LabelRule, 0, len(setRules)) | ||
| for _, rule := range setRules { | ||
| labelRules = append(labelRules, (*pd.LabelRule)(rule)) |
There was a problem hiding this comment.
Does changing the type of setRules(line157) to pd.LabelRule also eliminate the need for conversions?
There was a problem hiding this comment.
The main consideration here is that TiDB has made additional extensions to the SQL layer concept for the Rule type internally. Therefore, I decided to keep the relevant implementation of the Rule type in TiDB and hide the type conversion in the lower layer from the caller. Otherwise, we would have needed to completely replace Rule with pd.LabelRule, or constantly introduce type conversions throughout. However, prioritizing code readability over a slight performance sacrifice seems like a better choice.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, zimulala The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: ref #35319, tikv/pd#7300
Problem Summary:
Integrate PD HTTP client into the label manager.
What is changed and how it works?
LabelRuleManagerwith PD HTTP client.Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.